Conversation
WalkthroughThis update increases the minimum iOS and tvOS deployment targets to version 16, adds suite-level time limit annotations to all major test suites (mostly 2 minutes, 10 minutes for integration tests), and temporarily disables the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant CI
participant TestSuite
Developer->>CI: Push code / PR
CI->>CI: Build with warnings as errors
Note over CI: (swift test step is skipped/commented out)
alt Local or manual test run
Developer->>TestSuite: Run test suite
TestSuite-->>Developer: Enforce per-suite time limits (2 or 10 mins)
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/check.yaml (1)
95-97: Temporarily disable SPM tests with clear re-enable criteriaCommenting out the
swift teststep addresses CI hangs temporarily. Add a TODO or milestone reference (e.g., “Re-enable after #295 is resolved”) to ensure this workaround is revisited and removed once the root cause is fixed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/check.yaml(1 hunks)Package.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultChatClientTests.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/SubscriptionStorageTests.swift(1 hunks)
🔇 Additional comments (11)
Package.swift (1)
9-10: Ensure downstream compatibility when bumping iOS/tvOS deployment targetsBumping the minimum deployment targets to iOS 16 and tvOS 16 is a breaking change; please verify that any dependent projects or scripts (e.g., CI configs, Fastlane lanes, CocoaPods specs) are updated accordingly and update the README or other documentation if they reference older platform versions.
Tests/AblyChatTests/ChatAPITests.swift (1)
5-5: Add suite-level time limit for ChatAPITestsThe
@Suite(.timeLimit(.minutes(2)))annotation imposes a 2-minute cap on this suite. Ensure that all unit test suites use a consistent time limit and that integration tests retain the 10-minute limit.Tests/AblyChatTests/DefaultRoomTests.swift (1)
5-5: Add suite-level time limit for DefaultRoomTestsApplying
@Suite(.timeLimit(.minutes(2)))enforces a 2-minute global timeout. Confirm this aligns with other unit test files to maintain consistency across the test suite.Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
5-5: Add suite-level time limit for DefaultRoomReactionsTestsThe 2-minute suite-level timeout is correctly applied here. Please verify that all similar test suites have received the same annotation.
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
5-5: Suite-level time limit added – looks goodThe 2-minute cap is consistent with the other unit suites and will help surface hangs quickly without being overly strict.
Tests/AblyChatTests/SubscriptionStorageTests.swift (1)
4-4: Suite-level time limit added – looks goodThe new 2-minute limit aligns with the broader test policy and should not impact legitimate runs.
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1)
5-5: Suite-level time limit added – looks goodAdding a 2-minute ceiling is appropriate for this extensive but normally fast unit suite.
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (1)
5-5: Suite-level time limit added – looks goodMatches the project-wide policy; keeps runaway tests in check.
Tests/AblyChatTests/DefaultRoomsTests.swift (1)
5-5: Suite-level time limit added – looks goodConsistent 2-minute limit across unit suites is sensible and should aid CI stability.
Tests/AblyChatTests/DefaultChatClientTests.swift (1)
4-4: ```shell
#!/bin/bashSearch for all Xcode project pbxproj files to verify deployment targets
find . -type f -name project.pbxproj -exec rg -Hn IPHONEOS_DEPLOYMENT_TARGET '{}' + -exec rg -Hn TVOS_DEPLOYMENT_TARGET '{}' +
Locate Swift Package manifest(s)
find . -maxdepth 2 -type f -name Package.swift
</details> <details> <summary>Tests/AblyChatTests/IntegrationTests.swift (1)</summary> `13-13`: **Enforce a 10-minute timeout on integration tests** Extending `@Suite(.tags(.integration), .timeLimit(.minutes(10)))` prevents CI hangs during longer scenarios. As above, confirm your deployment targets are now iOS/tvOS 16.0+ to support `timeLimit`. You can reuse the same script from the unit tests check to validate the deployment targets. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@maratal there are all sorts of unrelated changes in this PR (changing iOS version, adding time limit); please can you remove? |
maratal
force-pushed
the
295-disable-SPM-tests-run
branch
from
a7945ef to
bab1712
Compare
done @lawrence-forooghian |
2 participants
Addresses #295
This will allow to stop tests hanging on CI until a better solution will show up.
Related PR - #297
Summary by CodeRabbit